feat: add range based exponential buckets in histogram #233
Conversation
Signed-off-by: Sidhant Kohli <[email protected]>
|
@mxinden please check! |
src/metrics/histogram.rs
Outdated
| if length < 1 { | ||
| panic!("ExponentialBucketsRange length needs a positive length"); | ||
| } |
There was a problem hiding this comment.
You could use a NonZeroU16 instead of a u16. That said, that would be inconsistent with exponential_buckets. Thoughts?
There was a problem hiding this comment.
I guess better to keep as u16 to keep consistent with exponential_buckets.
We can keep the additional check in the code for correctness.
What do you think?
src/metrics/histogram.rs
Outdated
| if min <= 0.0 { | ||
| panic!("ExponentialBucketsRange min needs to be greater than 0"); | ||
| } |
There was a problem hiding this comment.
We don't panic anywhere else in the crate. Reason being, that given that instrumentation is auxiliary only, folks likely don't want a panic. Would an empty Iterator work too?
There was a problem hiding this comment.
Sure! I have removed panics, but instead of an empty iterator I have added defaults with a warning.
Empty iterator might have have side-effects in histogram functionality.
Wdyt?
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
|
@mxinden Thanks for the review! Have addressed them with some small follow ups, please take a look whenever you get a chance. |
|
@mxinden Just following up on this? Let me know if any other changes are required |
Cargo.toml
Outdated
| @@ -24,6 +24,7 @@ parking_lot = "0.12" | |||
| prometheus-client-derive-encode = { version = "0.4.1", path = "derive-encode" } | |||
| prost = { version = "0.12.0", optional = true } | |||
| prost-types = { version = "0.12.0", optional = true } | |||
| log = "0.4.22" | |||
There was a problem hiding this comment.
I try to keep the dependency footprint of this crate as small as possible. Adding log just for this use-case is not an option. Mind reverting it?
|
Hey @mxinden Otherwise I'm good with having the empty iterator as return Let me know what works best and I can make those changes quickly and we can get this merged :) |
|
@mxinden Based on your latest addition to the change log file, could you please tell me what would be the conflict resolution here intended. |
Signed-off-by: Max Inden <[email protected]>
|
@kohlisid I resolved the merge conflicts. Sorry for the trouble. Please remove the |
Signed-off-by: Sidhant Kohli <[email protected]>
|
@mxinden Updated! Please check |
Signed-off-by: Max Inden <[email protected]>
The current implementation does not have a range based exponential bucket function.
This can be useful in scenarios where the end user doesn't want to perform manual calculations for extracting the required buckets, especially when multiple different ranges are to be used for different histograms.
Similar implementation is provided in the golang client as well.
https://github.com/prometheus/client_golang/blob/5d584e2717ef525673736d72cd1d12e304f243d7/prometheus/histogram.go#L125